feat(models): effectify ModelsDev as Service#25434
Merged
kitlangton merged 6 commits intodevfrom May 2, 2026
Merged
Conversation
Replaces the Promise-based, module-state-heavy ModelsDev with an
Effect-native Service that uses AppFileSystem (fs.stat / fs.readJson /
fs.writeWithDirs) and HttpClient (HttpClientRequest + filterStatusOk +
withTransientReadRetry) instead of raw fetch + Filesystem.* helpers.
Periodic refresh moves from a module-load setInterval to a scoped fork
that gets cleaned up with the Service's scope.
Effect-context callers yield ModelsDev.Service:
- src/provider/provider.ts (yields modelsDevSvc once at layer top)
- src/server/routes/instance/httpapi/handlers/provider.ts
- src/server/routes/instance/provider.ts
- src/cli/cmd/models.ts (drops the Effect.promise wrap)
Promise-context legacy callers (cli/cmd/github.ts, cli/cmd/providers.ts)
keep using ModelsDev.get() / ModelsDev.refresh(), which now route
through a tiny ManagedRuntime + Service wrapper that shares memoMap with
AppRuntime — so Effect callers and Promise callers see the same Service
instance and cache.
What stays Promise-wrapped:
- Flock.withLock for cross-process file coordination (in-process Effect
semaphores can't coordinate concurrent CLIs writing the same cache)
- The dynamic import('./models-snapshot.js') one-time bootstrap
Smoke tested: bun run dev models, bun run dev models nonexistent.
Provider + httpapi-provider tests pass (78/78). Typecheck clean.
Three review findings + a real bug:
1. Replace 'Effect.promise(() => Flock.withLock(... runPromise(fetchApi) ...))'
with 'Effect.scoped(Effect.gen { Flock.effect(lockKey); fetchAndWrite })'.
Flock.effect is the existing scoped acquire/release; the runPromise
re-entry inside the Promise lock body was dropping the parent fiber's
tracing span, scope, and interruption.
2. Replace hand-rolled 'ManagedRuntime.make(defaultLayer, { memoMap })' with
the existing 'makeRuntime(Service, defaultLayer)' helper from
src/effect/run-service.ts. Same pattern as bus, sync, instance-store, etc.
3. Use 'Effect.cachedInvalidateWithTTL(populate, Duration.infinity)' for
single-flight cache + manual invalidation. Concurrent first-callers now
share one populate; refresh() invalidates so the next get() re-populates.
4. Restore eager initial refresh — Schedule.fixed waits the duration before
the first run, which would have lost the original 'void refresh()' on
module load. Now: refresh().pipe(Effect.andThen(refresh().repeat(...))).
5. Simplify fresh() mtime check — fs.stat's info.mtime is always Option<Date>;
dropped the dead Option.isOption guard.
6. Drop unnecessary Effect.fnUntraced on loadFromDisk/loadSnapshot (these
are values, not traced effects). Replaced loadSnapshot's manual try/catch
wrapper with Effect.tryPromise + Effect.catch.
7. Effect.orDie on populate — Service interface declares get returns
Effect<Record<string, Provider>> (E = never); failures from HttpClient,
FileSystem, JSON.parse must die rather than silently leak.
Smoke + provider tests pass.
098a921 to
3a8a274
Compare
The new ModelsDev Service wasn't included in createRoutes layer mergeAll, so HttpApi handlers calling ModelsDev.Service.use() got an unhandled defect at runtime, surfacing as a bare 500.
hearnadam
reviewed
May 2, 2026
| function url() { | ||
| return Flag.OPENCODE_MODELS_URL || "https://models.dev" | ||
| export interface Interface { | ||
| readonly get: () => Effect.Effect<Record<string, Provider>> |
Contributor
Author
There was a problem hiding this comment.
ADAM! Great question. Effect.fn returns a function always, and we're using that for free log spans and some stack tracing biz. Yeah, a bit diff from ZIO patterns.
Contributor
Author
There was a problem hiding this comment.
Effects are still lazy and it doesn't have to be a thunk to work, but Effect.fn is actually pretty cool.
Mocks HttpClient via HttpClient.makeWith and pre-populates the on-disk cache to drive every code path: disk-hit, disk-empty fallback, single-flight dedup, in-memory cache stickiness, refresh fetch, fresh/stale TTL skip, and HTTP-error swallow. Layer.fresh is required to defeat the process-global MemoMap so each test gets its own cachedInvalidateWithTTL state.
- Use Schedule.spaced (not fixed) so the periodic refresh waits between completions instead of firing twice on startup. - Pull JSON.parse out of the Flock-held scope — keeps the cross-process lock release-eager. - Only invalidate after a successful fetchAndWrite; the no-op fresh-skip and ignored-failure paths shouldn't force the next get() to re-read. - Drop comments narrating obvious behavior; fix a misplaced WHY comment. - Use HttpClient.make (matches mockHttpClient elsewhere) instead of makeWith with a Preprocess cast in the new tests.
Mutating Flag.OPENCODE_MODELS_PATH at module-load time leaked the change into every subsequent test file in the same bun process — Provider state init then read undefined, fell into the snapshot/fetch fallback, and on slower CI lanes timed out or returned empty providers (surfacing as the "no providers found" failure on Windows for the SDK no-reply parity test). Wrap both mutations in beforeAll/afterAll so they only apply while this suite runs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacks on #25429. Replaces ModelsDev's Promise-based, module-state-heavy implementation with an Effect-native Service that uses `AppFileSystem` and `HttpClient` instead of raw `fetch` + `Filesystem.*` helpers. The periodic refresh moves from a module-load `setInterval` to a scoped fork.
What's now Effect-native
What stays Promise-wrapped
Effect-context callers converted
Promise-context legacy callers
`cli/cmd/github.ts` and `cli/cmd/providers.ts` (still vanilla `async (args) => ...` yargs handlers, not yet on `effectCmd`) keep calling `ModelsDev.get()` / `ModelsDev.refresh()`. Those are now thin `AppRuntime.runPromise(Service.use(...))` wrappers using the shared memoMap — so the Service instance and cache are the same one Effect callers see.
Followup notes
Tests